Code review и конфликт в динамике команды

Команды программистов из 3-7 человек это идеальная машина по быстрому созданию качественного продукта. Слишком много - и все погрязнут в бесконечных обсуждениях, слишком мало - будет сбиваться ритм и снижаться продуктивность и качество.

Я мало что понимаю в менеджменте, поэтому меня больше интересуют вопросы конфликтов и улучшение инспекции кода для улучшения продукта и сплочения команды.

Code review, pull request review и peer review это практически одно и то же, различие лишь в фокусе внимания - на систематичность просмотра существующего кода, на принятие чужого кода или на массовость читателей. Я буду использовать термин ревью как процесса и инспектора для принимающей стороны.

Автоматизация скучного

Самое очевидное что надо сделать для команды - убрать дебаты о стиле переменных, кавычек, количестве пробелов и границе сложности функций. Это всё должны делать автоматические инструменты до коммита

• Style guide - надо иметь свои настройки для phpstorm + так же настроенный phpcs, codeship

• Linters - jslint, phpmd

• Complexity - sonar

• Test coverage

Хороший pull-request

Если вы предлагаете код для внесения в репозиторий своей команды, имеет смысл описать его контекст, что-бы сразу было наглядно видно

• ​Номер тикета в Jira

• Требования (скопированные из тикета)

• Скриншоты / gif анимация наглядных UI-изменений

• Зависимости от других pull-request'ов и порядок их слияния

• Заметки на конкретных строках где вы не уверены или хотите обратить внимание команды

Размер изменений должен быть достаточно мал что-бы ревью не превышало часа и 500 строк кода. При разработке фичи, размер рефакторинга побочных функций не должно занимать более ~ 20% от всех изменений.

Хороший ревью

Процесс принятия кода в репозиторий должен начинаться сразу после готовности pull-request'а. Это не даст автору и коду остыть и впасть в конфликт с новыми запросами.

Ревью проходит в плане абстракций сверху вниз и по порядку проверяет

• Выполнены ли требования, обрабатываются ли ошибки

• Архитектурную согласованность — API, DB

• Нефункциональные требования

• Читаемость - именование переменных, порядок аргументов

• Сложность - принцип single responsibility, иерархия вызова функций, повторное использование

• Безопасность - инъекции, передача личных данных

• Performance - количество запросов к сети, БД, узкие места от количества данных, асинхронные race conditions

Критика и язык

Пожалуй самое сложное в ревью это не обидеть автора, не снизить его самооценку и не вызвать личную неприязнь

• Используйте пассивную форму общения (вопросы и предложения), а не повелительное наклонение (приказы и требования)

• Избегайте обобщений (всегда, никогда, все)

• Критикуйте код, а не автора - избегайте ярлыков, осуждения и перехода на личности

• Не перекладывайте на разработчика слишком много вопросов (а что будет, если..) где вы подозреваете Edge case. Проверьте сами и поставьте его перед фактом

• Чётко указывайте степень своей уверенности (факт - вероятность - мнение) и степень внимания (критичность) комментария

• Проявляйте заботу при критике - предлагайте псевдокод, ссылки на строки кода или обучение. 

• Знайте когда перестать устраивать чат в гитхабе/битбакете

• Если всё тлен - подходите физически и программируйте парно

Типы комментариев

Поскольку я постоянно занимаюсь ревью, а на один pull-request у меня обычно уходит 5-20 комментариев, то я специально вначале пишу тип комментария, что-бы автору сразу было понятно на что стоит обратить внимание сразу

Major	дефект, нечто явно не работающее, вызывающее ошибку
Medium	какой-то случай не обрабатывается или не по-стандарту, загрязнение объекта переменными, загрязнение функции низкоуровневыми функциями, нарушение state из-за race conditions
Edge case	довольно редкий не обрабатуемый случай, надо чётко высказывать обязательно ли его исправлять, или решать автору
Naming	субъективное мнение о читаемости переменных, функций, исключений
Out of scope	возможное улучшение, которое явно не стоит делать в рамках этой задачи (например UI handling под mobile)
Optional	альтернативное решение (другая функция, другой порядок и тп.) о которое не обязательно обсуждать
Minor	мелкая проблема со стилем кода, опечатка, ненужная переменная или вызов функции, отсутсвующий jsdoc, покрытие тестами
Debateable	спорный момент в котором инспектор не уверен и хотелось бы поднять обсуждение команды
Security vulnerability	потенциальная уязвимость, утечка или повреждение данных, DoS
Note	обращение внимания, не требует изменений
Performance	неоптимальные запросы, алгоритмическая сложность
Improvement	небольшое улучшение которое можно было бы сделать в этой задаче
Redundant	лишний код
Like	похвала. Хорошее решение, радость от рефакторинга или использование доселе неизвестной мне функции
I donno	я не понимаю что этот код делает / незнаю синтаксиса
Why	стандартный вопрос когда не понимаю зачем это надо

Спорные моменты

Должен ли инспектор проводить поиск дефектов, т.е. проводить QA?

Я склоняюсь к тому что да, инспектор должен локально воспроизвести состояние нового продукта и полностью его протестировать. Однако если инспекторов несколько и они делают проверку одновременно, а при этом коммитятся изменения несколько раз подряд, это сильно растягивает процесс и вносит неразбериху

Сколько человек должно проводить инспекцию и кто это должен быть?

Самое быстрое и эффективное решение - один человек кто сейчас может. Но я склоняюсь к тому что ревью должна делать вся команда. Pull request обязательн получить минимум 2 одобрения, после чего полноценную установку с поиском дефектов делать не обязательно

Стратегии выбора инспектора

В разных компаниях и командах выбор смотрящего происходит по-разному, зависит от ситуации:

• всегда выбирать ближайшего старшего разработчика (тим лид) — подходит если много новичков

• всегда выбирать более компетентного — подходит если вы коммитите в чужой репозиторий

• по специализации (бэкэнд, знанию фреймворка, БД)

• по доменной области / коду (по частоте работы с конкретным файлом)

• broadcast, оповещение всех, кто свободен тот и глянет — подходит если нет жёсткого контроля, но могут быть проблемы с провисанием ревью из-за низкой мотивации

• контрольный центр — старший делегирует ревью конкретному человеку (диктатура). Получится быстро, но сильно зависит от логики центра

• контракт (пара) на спринт — может вызвать губительное привыкание

• эксперт со стороны — полезно для редких ситуаций

Конфликт

Конфликты в команде это благо. Это показатель здоровья и конкуренции мнений.

Конфликты возникают из-за ценностей или из-за процесса.

Например, если мы спорим из-за названия переменных и функций, то это нефункциональная ценность читаемости, т.е. поддерживаемости проекта. Если этот спор длится слишком долго, возникает лагерь для которых важна скорость разработки. То же может происходить из-за конфликтов безопасности и UX, масштабируемости и консистентности. Это конфликты ценностей.

Если у нас scrum и PM описывает задачу используя обобщения ("сделать чат как в фейсбуке") вместо детального описания, то разработчику прийдётся самому исследовать и формализовать требования вместо PM, что ему может не понравиться. Аналогично, разработчики могут не делать глубокого поиска дефектов, считая это ответственностью автора, или не заниматься поддержкой live среды, считая это работой operation engineers. Это конфликты процессов.

Лучший способ разрешения конфликта это выговориться. В Agile для этого есть ретро-митинг за неделю, где можно письменнно и аргументированно выплеснуть свои эмоции. Это даёт возможность начальству отслеживать состояние команды, скажем на протяжении года.

Худший вариант это подавление эмоций и удаление от конфликта и безразличие — «ну хорошо раз Петя хочет так, то пусть так, а если Вася хочет по-другому, то пускай сами разберутся, а я сделаю как они решат»

Личный опыт

Поскольку я конфликтный сам по себе, поднятие проблем достаточно простое дело. Но всё-таки приятно когда конфликт успешно разрешается так что обе стороны довольны

Конфликты ценностей

• ​Шумности в офисе (концентрация vs общение)
• Наличие bus factor в команде (специализация и глубина vs масштабируемость)
• Фокус на одном типе тестов (отслеживаемость и простота) vs разнотипные баги (эффективность)
• Однородные микросервисы (надёжность) vs изучение новых, более эффективных фреймворков (развитие)
• Централизация данных для разработки (управление) vs локализация и усложнение обновлений (надёжность)

Конфликты процессов

• Глубинное планирование vs решения на ходу

• Внесение изменений в ходе спринта vs гибкость разработки

• Напряг с поиском инспектора кода vs свобода разработчиков в планировании своего времени

• Зависимость задач между собой vs возможная параллельность

• Медленной реакции / общении между командами vs фокус на своих продуктах

• Рост технического долга vs быстрая ранняя скорость разработки

• Ломающийся лок зависимостей на час vs долгий и болезненный апгрейд всех сервисов

• Стандартизация UI компонентов vs трата времени и денег при редком использовании

Личностные особенности

Катализатором конфликтов могут быть межличностные отношения. Люди по-разному относятся к общению. Я больше формалист и поскольку память у меня никакая, приходится много сводить к записям и им я доверяю больше. 

Другие люди имеют свои недостатки — кто-то наобещает всего и забудет. Кто-то медленно продвигается, но всех опрашивает, кто-то двигается быстро, набивает шишки и матерится из-за этого, а кто-то сидит тихо и не любит критику, кто-то двигается медленно потому что не любит спрашивать помощи..

Но именно готовность к открытому конфликту — важное условие, чтобы стать полноценной рабочей группой. Команда сначала должна подраться. Иначе к решению быстрых, сложных задач она не способна.

Павел Вейник

Отличие от парного программирования

Парное программирование это ревью кода в реальном времени. Но есть нюансы:

• После парного программирования не остаётся письменной истории. Остальная команда получает уже хороший код и поэтому может медленней учиться на чужих ошибках
• Парное программирование физически синхронное. Это значит что ревью нельзя делать в другом пространстве-времени. Я частенько просматриваю код из дому, повышая свою эффективность

Несомненно от него огромные плюсы по глубине охвата, скорости дебага и качеству кода.

Польза

Ревью необходимы команде, потому что оно:

• Улучшает и украшает код

• Находит альтернативные, элегантные способы решения

• Передаёт знания о продукте

• Увеличивает владение и повторное использование кода

• Информирование команду об изменениях и поднимает дискуссии

• Разрешает конфликты через консенсус, снижая негатив при этом

• Обучает разработчиков

• Снижает число дефектов

• Даёт быструю обратную связь для ранних прототипов

• Создаёт историю причин и следствий

• Позволяет оценивать работу команды через сбор метрик

См. также

• Code Review Etiquette
• How to Do Code Reviews Like a Human
• May the Code Review be with you
• Конфликты команды разработчиков
• Top 10 obstacles to faster code review
• Gitlab code review guidelines
• Refactoring guru